The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).

Summary of Changes

Hello @julienrbrt, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request enhances the node synchronization mechanism by introducing a "trusted height" feature. This allows nodes to bootstrap their state from a known, verified block height, significantly reducing the time required for initial synchronization, especially in long-running networks. The changes encompass new configuration options, robust validation to ensure secure usage, and integration into the core synchronization and state initialization processes.

Highlights

• Trusted Height Synchronization: Introduced the ability for nodes to start synchronization from a specified "trusted height" and corresponding header hash, rather than always from genesis.
• Configuration Parameters: Added TrustedHeight and TrustedHeaderHash to the P2P configuration, allowing users to define a specific starting point for synchronization.
• Security Validation: Implemented validation to ensure that if a TrustedHeight is provided, a TrustedHeaderHash must also be supplied to prevent syncing from an unverified or malicious state.
• Core Sync Logic Integration: Modified the Syncer and SyncService components to incorporate the trusted height logic, including fetching and verifying the trusted header from the store and P2P, and initializing the chain state accordingly.
• Documentation Update: Updated the docs/learn/config.md to provide comprehensive details, examples, and security considerations for the new trusted height synchronization feature.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Claude finished @julienrbrt's task in 1m 54s —— View job

Code Review: Trusted Height Sync Feature

I've completed a comprehensive review of this PR. The trusted height sync feature is well-designed and addresses a real need for faster node bootstrapping. However, I've identified several critical issues that need attention, along with recommendations for security, error handling, and testing improvements.

Critical Issues

1. execReplayer.SyncToHeight Incompatibility with Trusted Height ⚠️ CRITICAL

Location: block/internal/syncing/syncer.go:395-401

Gemini's review identified this correctly. When initializing from a trusted height, the code calls s.exec.InitChain() with the trusted header's height (line 322-330), but then unconditionally calls execReplayer.SyncToHeight(ctx, state.LastBlockHeight) at line 397.

Problem: The replayer is initialized with genesis information and expects to replay blocks sequentially from genesis.InitialHeight. When syncing from trusted height 100000, the store won't have blocks 1-99999, causing GetBlockData to fail in the replayer.

Fix: Skip the replayer sync when initializing from trusted height:

// Sync execution layer with store on startup
// Skip replay if we initialized from trusted height since executor is already at that state
if s.config.P2P.TrustedHeight == 0 || state.LastBlockHeight != s.config.P2P.TrustedHeight {
    execReplayer := common.NewReplayer(s.store, s.exec, s.genesis, s.logger)
    if err := execReplayer.SyncToHeight(s.ctx, state.LastBlockHeight); err != nil {
        return fmt.Errorf("failed to sync execution layer on startup: %w", err)
    }
}

Fix this issue →

2. Documentation Inconsistency ⚠️ HIGH PRIORITY

Location: docs/learn/config.md:1000-1023

The documentation shows the configuration under a sync: section with --evnode.sync.* flags, but the actual implementation places these fields in P2PConfig with --evnode.p2p.* flags.

Current (incorrect) documentation:

sync:
  trusted_height: 100000
  trusted_header_hash: "a1b2c3d4e5f6..."

Should be:

p2p:
  trusted_height: 100000
  trusted_header_hash: "a1b2c3d4e5f6..."
  trusted_data_hash: "d7e8f9g0h1i2..."

Command-line flags should be:

• --evnode.p2p.trusted_height
• --evnode.p2p.trusted_header_hash
• --evnode.p2p.trusted_data_hash

Also note the documentation is missing trusted_data_hash entirely, which is required by the validation logic.

Security Issues

3. Hash Verification Logic Has Potential Security Gaps ⚠️ MEDIUM

Location: pkg/sync/sync_service.go:442-447

The hash verification uses OR logic, accepting if the hash matches EITHER trustedHeaderHash OR trustedDataHash:

if actualHash != syncService.trustedHeaderHash && actualHash != syncService.trustedDataHash {
    return fmt.Errorf("trusted header hash mismatch...")
}

Issues:

• For header sync service, why would we check against trustedDataHash?
• For data sync service, why would we check against trustedHeaderHash?
• The OR logic means if only ONE hash is provided, ANY entity hash would pass if it matches that single hash

Recommendation: The sync service type should determine which hash to validate:

var expectedHash string
if syncService.syncType == headerSync {
    expectedHash = syncService.trustedHeaderHash
} else {
    expectedHash = syncService.trustedDataHash
}

if expectedHash == "" {
    return fmt.Errorf("no trusted hash configured for %s", syncService.syncType)
}

actualHash := trusted.Hash().String()
if actualHash != expectedHash {
    return fmt.Errorf("trusted %s hash mismatch at height %d: expected %s, got %s",
        syncService.syncType, syncService.trustedHeight, expectedHash, actualHash)
}

4. No Hash Format Validation ⚠️ MEDIUM

Location: pkg/config/config.go:386-390

The validation only checks that the hash strings are non-empty, but doesn't validate the format (e.g., hex encoding, expected length).

Recommendation: Add hash format validation to catch user errors early:

if c.P2P.TrustedHeight > 0 {
    if c.P2P.TrustedHeaderHash == "" || c.P2P.TrustedDataHash == "" {
        return fmt.Errorf("trusted_height (%d) requires both trusted_header_hash and trusted_data_hash", c.P2P.TrustedHeight)
    }
    
    // Validate hash format (assuming SHA256 hex encoding = 64 chars)
    if len(c.P2P.TrustedHeaderHash) != 64 {
        return fmt.Errorf("trusted_header_hash must be 64 hex characters (got %d)", len(c.P2P.TrustedHeaderHash))
    }
    if len(c.P2P.TrustedDataHash) != 64 {
        return fmt.Errorf("trusted_data_hash must be 64 hex characters (got %d)", len(c.P2P.TrustedDataHash))
    }
    
    // Validate hex encoding
    if _, err := hex.DecodeString(c.P2P.TrustedHeaderHash); err != nil {
        return fmt.Errorf("trusted_header_hash must be valid hex: %w", err)
    }
    if _, err := hex.DecodeString(c.P2P.TrustedDataHash); err != nil {
        return fmt.Errorf("trusted_data_hash must be valid hex: %w", err)
    }
}

Error Handling & Resilience Issues

5. No Retry Logic for Trusted Header Fetch ⚠️ MEDIUM

Location: pkg/sync/sync_service.go:352-354

The trusted header fetch fails immediately without retry logic, even though the subsequent P2P initialization in initFromP2PWithRetry has exponential backoff (lines 398-428).

Recommendation: Wrap the trusted header fetch in the same retry logic, or incorporate it into the tryInit function's retry loop.

6. Trusted Header Fetch Error Loses Context ⚠️ LOW

Location: pkg/sync/sync_service.go:437-440

When ex.GetByHeight fails, the error message doesn't indicate whether this is a network issue, peer unavailability, or hash mismatch.

Recommendation: Add more context to help operators diagnose issues:

trusted, err := syncService.ex.GetByHeight(ctx, syncService.trustedHeight)
if err != nil {
    return fmt.Errorf("failed to fetch trusted header at height %d from P2P peers (ensure peers are available and synced past this height): %w", 
        syncService.trustedHeight, err)
}

Code Quality & Architecture

7. Unused Parameter Should Be Removed ⚠️ LOW

Location: pkg/sync/sync_service.go:433

Gemini correctly identified that peerIDs []peer.ID is declared but never used in fetchAndVerifyTrustedHeader. The call site at line 352 passes it unnecessarily.

Fix:

func (syncService *SyncService[H]) fetchAndVerifyTrustedHeader(ctx context.Context) error {

And update the call site to remove the argument.

8. Missing Trusted Height in Store Initialization ⚠️ LOW

Location: block/internal/syncing/syncer.go:332-342

When initializing state from trusted height, the code sets state.DAHeight = s.genesis.DAStartHeight. This might be incorrect if the trusted height is significantly ahead of genesis - the DA height should probably be aligned with where the trusted block was posted.

Consideration: Should we also require a TrustedDAHeight configuration parameter? Or query it from the trusted block metadata?

9. State Version Inconsistency ⚠️ LOW

Location: block/internal/syncing/syncer.go:332-342 vs 355-362

When initializing from trusted height, the code sets state.Version = types.InitStateVersion (line 333), but when initializing from genesis it doesn't set any version. This inconsistency could cause issues.

Recommendation: Ensure version is set consistently in both paths.

Testing Coverage Concerns

10. Low Test Coverage (36.48%) ⚠️ HIGH PRIORITY

Codecov reports only 36.48% coverage with 47 lines missing coverage. Critical untested paths include:

block/internal/syncing/syncer.go: 22 missing + 2 partials

• Trusted header loading and verification (lines 308-342)
• Error paths in trusted height initialization
• Edge cases like hash mismatch, missing header

pkg/sync/sync_service.go: 23 missing lines

• fetchAndVerifyTrustedHeader function (lines 433-460)
• Hash verification logic
• Store append failures

Recommendation: Add integration tests covering:

1. Successful sync from trusted height
2. Hash mismatch scenarios
3. Trusted header not available in P2P store
4. Trusted height with missing/corrupted data
5. Trusted height = 0 (normal genesis sync)
6. Config validation for trusted height parameters

11. Existing Test Update Needed ⚠️ MEDIUM

Location: pkg/config/config_test.go:115

The test expects exactly 66 flags, but the PR adds 3 new flags (TrustedHeight, TrustedHeaderHash, TrustedDataHash). The flag count assertion will fail or needs updating to 69.

Documentation & User Experience

12. Missing Security Warnings in Documentation

The documentation should include prominent security warnings:

• ⚠️ Trust assumption: Users must obtain the trusted hash from a reliable source (e.g., block explorer, trusted node operator)
• ⚠️ Security trade-off: Syncing from trusted height reduces security guarantees - the node trusts that height without verifying all prior blocks
• ⚠️ When to use: Suitable for non-critical nodes, testnets, or when combined with other verification methods
• ⚠️ Not for validators: Validator nodes should sync from genesis for full verification

13. Missing Operational Guidance

Documentation should explain:

• How to obtain the trusted height and hash (example using block explorer or RPC)
• What happens if the hash is wrong (node will fail to start with clear error)
• How to fall back to genesis sync if needed (remove the flags)
• Performance expectations (e.g., "Syncing from height 100k instead of genesis reduces sync time from X hours to Y minutes")

Positive Aspects ✅

1. Clean separation of concerns - Config validation, P2P fetching, and state initialization are well-separated
2. Good logging - Informative log messages at key points (lines 310, 434, 449-451)
3. Validation at config level - Catches misconfiguration before node startup
4. Works with existing architecture - Integrates cleanly with P2P sync and store initialization
5. Test coverage for config validation - TestTrustedHeightValidation covers basic validation scenarios

Summary & Recommendations

This PR implements a valuable feature that will significantly improve user experience for node operators. However, the critical issue with execReplayer.SyncToHeight must be fixed before merging, as it will cause nodes to fail when using trusted height sync.

Before Merging:

1. ✅ Fix the replayer sync issue (Critical)
2. ✅ Correct the documentation (High priority)
3. ✅ Improve hash verification logic (Security)
4. ✅ Add hash format validation (Security)
5. ✅ Add integration tests for trusted height scenarios (High priority)
6. ✅ Remove unused peerIDs parameter (Code quality)

Nice to Have:

• Add retry logic for trusted header fetch
• Improve error messages with more context
• Add security warnings to documentation
• Consider requiring TrustedDAHeight parameter
• Add operational guidance for users

Overall Assessment: The architecture and approach are sound, but critical bugs and security improvements are needed before this can be safely deployed.

Codecov Report

❌ Patch coverage is 36.00000% with 48 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.18%. Comparing base (d20b1ac) to head (8a0e65d).
⚠️ Report is 1 commits behind head on main.

@@            Coverage Diff             @@
##             main    #3050      +/-   ##
==========================================
- Coverage   56.38%   56.18%   -0.20%     
==========================================
  Files         118      118              
  Lines       12036    12093      +57     
==========================================
+ Hits         6787     6795       +8     
- Misses       4507     4555      +48     
- Partials      742      743       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Closing, as we'll still need snapshots for ev-reth, which does not improve the UX.